ndpi: ndpi as a plugin - v5 #12423

jasonish · 2025-01-17T22:25:03Z

Previous PR: #12412

Changes:

Reference ticket for C name collisions
Break up keyword registration and keyword ID assignment into 2 commits. This plugin just needs the dynamic ID assignment.

Outstanding comment from previous PR:

I guess that last bit we be an S-V test, since we do plan on shipping this in our tree.

Ticket: OISF#7231

- Download and build nDPI - Enable nDPI during Suricata ./configure - Test that the plugin was built and installed

The format is left free-form, as its controled by a plugin.

By adding no_mangle to non-pub functions they enter the global name space and can conflict with other functions. In this case another sha library used by a plugin. Related ticket: https://redmine.openinfosecfoundation.org/issues/7498

The eve callback in ndpi requires a flow, so bail earlier if there is no flow.

Split DetectHelperKeywordRegister into 2 functions, one for acquiring a new keyword ID, and another to perform the registration. This makes it easier to do the traditional C keyword initialization with a dynamic ID.

codecov · 2025-01-17T22:57:37Z

Codecov Report

Attention: Patch coverage is 92.30769% with 2 lines in your changes missing coverage. Please review.

Project coverage is 80.63%. Comparing base (8f6795d) to head (b7fa276).

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #12423      +/-   ##
==========================================
- Coverage   80.63%   80.63%   -0.01%     
==========================================
  Files         918      918              
  Lines      258696   258696              
==========================================
- Hits       208598   208587      -11     
- Misses      50098    50109      +11

Flag	Coverage Δ
fuzzcorpus	`56.81% <92.30%> (+<0.01%)`	⬆️
livemode	`19.40% <92.30%> (+<0.01%)`	⬆️
pcap	`44.27% <92.30%> (-0.03%)`	⬇️
suricata-verify	`63.25% <92.30%> (+0.01%)`	⬆️
unittests	`58.52% <92.30%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

suricata-qa · 2025-01-18T01:00:03Z

Information: QA ran without warnings.

Pipeline 24257

victorjulien

see inline

victorjulien · 2025-01-18T09:58:40Z

doc/userguide/rules/ndpi-protocol.rst

+Suricata should be compiled with the nDPI support and the ``ndpi`` 
+plugin must be loaded before it can be used. 
+
+Example of configuring Suricata to be compiled with nDPI support:


would be good to mention the requires rule support here as well I think

victorjulien · 2025-01-18T09:59:05Z

doc/userguide/rules/ndpi-risk.rst

+- Malware host contacted
+- and many other...
+
+Suricata should be compiled with the nDPI support and the ``ndpi`` 


requires support

victorjulien · 2025-01-18T10:00:29Z

plugins/ndpi/ndpi.c

+                    flowctx->detection_completed = 1;
+            }
+        } else {
+            u_int16_t max_num_pkts = (f->proto == IPPROTO_UDP) ? 8 : 24;


victorjulien · 2025-01-18T10:01:19Z

plugins/ndpi/ndpi.c

+            u_int16_t max_num_pkts = (f->proto == IPPROTO_UDP) ? 8 : 24;
+
+            if ((f->todstpktcnt + f->tosrcpktcnt) > max_num_pkts) {
+                u_int8_t proto_guessed;


uint8_t

Needs a full scan for the u_ variant of int types. We use the one w/o underscore.

victorjulien · 2025-01-18T10:01:59Z

plugins/ndpi/ndpi.c

+struct NdpiFlowContext {
+    struct ndpi_flow_struct *ndpi_flow;
+    ndpi_protocol detected_l7_protocol;
+    uint8_t detection_completed;


used as a bool, so should be a bool

victorjulien · 2025-01-18T10:05:25Z

plugins/ndpi/ndpi.c

+{
+    DetectnDPIProtocolData *data = NULL;
+
+    data = DetectnDPIProtocolParse(arg, s->init_data->negated);


declare data here

victorjulien · 2025-01-18T10:07:02Z

plugins/ndpi/ndpi.c

+        SCReturnInt(0);
+    }
+
+    r = ((flowctx->ndpi_flow->risk & data->risk_mask) == data->risk_mask);


declare r here

victorjulien · 2025-01-18T10:07:51Z

plugins/ndpi/ndpi.c

+{
+    DetectnDPIRiskData *data = NULL;
+
+    data = DetectnDPIRiskParse(arg, s->init_data->negated);


declare data here

victorjulien · 2025-01-18T10:08:17Z

plugins/ndpi/ndpi.c

+    struct NdpiFlowContext *flowctx = FlowGetStorageById(f, flow_storage_id);
+    ndpi_serializer serializer;
+    char *buffer;
+    u_int32_t buffer_len;


victorjulien · 2025-01-18T10:10:33Z

plugins/ndpi/ndpi.c

+    FlowSetStorageById(f, flow_storage_id, flowctx);
+}
+
+static void OnFlowUpdate(ThreadVars *tv, Flow *f, Packet *p, void *_data)


is it a problem that this gets the raw data in the order as it comes in? So there will be no stream reassembly, reordering and normalization of any of the data sent to ndpi.

Getting data as it comes in is fine for ndpi

jasonish · 2025-01-18T16:45:22Z

Ping @cardigliano, can you take a look at these issues? I think the "integration" is more or less complete with this PR, just some code items to deal with in the implementation of the plugin.

cardigliano · 2025-01-20T10:00:01Z

I fixed the issues in our branch

cardigliano and others added 6 commits January 17, 2025 14:32

ndpi: initial implementation of nDPI plugin

3f5f16d

Ticket: OISF#7231

github-ci: add ndpi build to the centos-stream9 build

2079242

- Download and build nDPI - Enable nDPI during Suricata ./configure - Test that the plugin was built and installed

eve/schema: add top level ndpi object

b36597e

The format is left free-form, as its controled by a plugin.

ndpi: check for flow earlier in eve callback

70984d8

The eve callback in ndpi requires a flow, so bail earlier if there is no flow.

detect: split new keyword id from registration

b7fa276

Split DetectHelperKeywordRegister into 2 functions, one for acquiring a new keyword ID, and another to perform the registration. This makes it easier to do the traditional C keyword initialization with a dynamic ID.

jasonish requested review from jufajardini, victorjulien and a team as code owners January 17, 2025 22:25

jasonish mentioned this pull request Jan 17, 2025

ndpi: ndpi as a plugin - v4 #12412

Closed

victorjulien requested changes Jan 18, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ndpi: ndpi as a plugin - v5 #12423

ndpi: ndpi as a plugin - v5 #12423

jasonish commented Jan 17, 2025

codecov bot commented Jan 17, 2025

suricata-qa commented Jan 18, 2025

victorjulien left a comment

victorjulien Jan 18, 2025

victorjulien Jan 18, 2025

victorjulien Jan 18, 2025

victorjulien Jan 18, 2025

victorjulien Jan 18, 2025

victorjulien Jan 18, 2025

victorjulien Jan 18, 2025

victorjulien Jan 18, 2025

victorjulien Jan 18, 2025

victorjulien Jan 18, 2025

cardigliano Jan 20, 2025

jasonish commented Jan 18, 2025

cardigliano commented Jan 20, 2025

ndpi: ndpi as a plugin - v5 #12423

Are you sure you want to change the base?

ndpi: ndpi as a plugin - v5 #12423

Conversation

jasonish commented Jan 17, 2025

codecov bot commented Jan 17, 2025

Codecov Report

suricata-qa commented Jan 18, 2025

victorjulien left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasonish commented Jan 18, 2025

cardigliano commented Jan 20, 2025